Conversation
1. Only need to split slice when post ckpt size is greater than the current size of the slice and greater than the slice upper bound. 2. For non snapshot read, there is no need to return the deleted keys from the data store.
WalkthroughThese changes modify snapshot-aware filtering and slice-splitting logic across store handler and transaction service components. Range slice splitting conditions are tightened in template maps and local shards, while snapshot-aware filtering is introduced to skip deleted or out-of-date entries during range reads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @store_handler/data_store_service_client_closure.cpp:
- Around line 1317-1328: The snapshot filtering condition is inverted: inside
the if-block that checks (snapshot_ts == 0 && is_deleted) || (snapshot_ts > 0 &&
snapshot_ts > ts) you should flip the comparison to (ts > snapshot_ts) so it
reads (snapshot_ts == 0 && is_deleted) || (snapshot_ts > 0 && ts > snapshot_ts);
update that condition in the same if used for skipping deleted/newer records
(variables snapshot_ts, ts, is_deleted) so the code matches the comment and MVCC
semantics (skip records newer than snapshot) and retains the existing
continue/backfill behavior.
🧹 Nitpick comments (1)
tx_service/include/cc/template_cc_map.h (1)
5424-5428: Slice‑split predicate matches intent; consider syncing comments and minor cleanupThe new condition
return (slice->PostCkptSize() > StoreSlice::slice_upper_bound && slice->PostCkptSize() > slice->Size());correctly implements “split only when post‑ckpt size exceeds both the slice upper bound and the current slice size” as described in the PR.
Two small follow‑ups:
- The RangePartitionDataSyncScanCc block comment above (around Line 5196) still describes splitting only in terms of
PostCkptSizevsupper_boundand can now be misleading. It would be good to update it to mention the> current sizerequirement (e.g., “PostCkptSize > max(slice_upper_bound, current_size)”).- For readability and to avoid repeated calls, you could cache
PostCkptSize()in a localauto post_ckpt_size = slice->PostCkptSize();and use that in the comparisons and theassert.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
store_handler/data_store_service_client_closure.cpptx_service/include/cc/template_cc_map.htx_service/src/cc/local_cc_shards.cpp
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-20T04:30:07.884Z
Learnt from: liunyl
Repo: eloqdata/tx_service PR: 149
File: include/cc/cc_request.h:1876-1927
Timestamp: 2025-10-20T04:30:07.884Z
Learning: ScanNextBatchCc in include/cc/cc_request.h is used only for hash-partition scans; range-partition scans are handled by ScanSliceCc.
Applied to files:
tx_service/include/cc/template_cc_map.h
📚 Learning: 2025-11-11T07:10:40.346Z
Learnt from: lzxddz
Repo: eloqdata/tx_service PR: 199
File: include/cc/local_cc_shards.h:233-234
Timestamp: 2025-11-11T07:10:40.346Z
Learning: In the LocalCcShards class in include/cc/local_cc_shards.h, the EnqueueCcRequest methods use `shard_code & 0x3FF` followed by `% cc_shards_.size()` to distribute work across processor cores for load balancing. This is intentional and separate from partition ID calculation. The 0x3FF mask creates a consistent distribution range (0-1023) before modulo by actual core count.
Applied to files:
tx_service/include/cc/template_cc_map.htx_service/src/cc/local_cc_shards.cpp
📚 Learning: 2025-12-02T10:43:27.431Z
Learnt from: lokax
Repo: eloqdata/tx_service PR: 254
File: tx_service/src/cc/local_cc_shards.cpp:2949-3188
Timestamp: 2025-12-02T10:43:27.431Z
Learning: In tx_service/src/cc/local_cc_shards.cpp, whenever TryPinNodeGroupData is used, only call Sharder::Instance().UnpinNodeGroupData(node_group) if the recorded term is >= 0 (i.e., pin succeeded). Example: LocalCcShards::PostProcessFlushTaskEntries guards the unpin with `if (term >= 0)`.
Applied to files:
tx_service/src/cc/local_cc_shards.cpp
📚 Learning: 2025-10-09T03:56:58.811Z
Learnt from: thweetkomputer
Repo: eloqdata/tx_service PR: 150
File: include/cc/local_cc_shards.h:626-631
Timestamp: 2025-10-09T03:56:58.811Z
Learning: For the LocalCcShards class in include/cc/local_cc_shards.h: Writer locks (unique_lock) should continue using the original meta_data_mux_ (std::shared_mutex) rather than fast_meta_data_mux_ (FastMetaDataMutex) at this stage. Only reader locks may use the FastMetaDataMutex wrapper.
Applied to files:
tx_service/src/cc/local_cc_shards.cpp
🧬 Code graph analysis (1)
store_handler/data_store_service_client_closure.cpp (1)
tx_service/src/cc/cc_shard.cpp (1)
key(1327-1327)
🔇 Additional comments (3)
store_handler/data_store_service_client_closure.cpp (2)
1279-1279: LGTM: Snapshot timestamp extraction.Extracting the snapshot timestamp for use in filtering logic is appropriate.
1285-1289: LGTM: Advancing start key for next batch.Updating
kv_start_key_when processing the last item correctly prepares the next batch's start position, even if that item is subsequently skipped by the filtering logic.tx_service/src/cc/local_cc_shards.cpp (1)
5465-5469: Slice-split gating now correctly requires growth beyond both thresholdsThe updated guard (
slice_post_ckpt_size <= StoreSlice::slice_upper_bound || slice_post_ckpt_size <= curr_slice->Size()) means we only enter the split path whenPostCkptSizeis finite, aboveslice_upper_bound, and strictly larger than the pre-ckpt slice size. This matches the PR’s stated heuristic and is consistent with the paused-slice handling earlier inUpdateSlices(any slice that previously entered the split path already hadPostCkptSize > Size()). I don’t see correctness or concurrency issues here; just ensure tests cover cases where slices shrink but remain aboveslice_upper_boundso this behavior is intentional.
| if ((snapshot_ts == 0 && is_deleted) || | ||
| (snapshot_ts > 0 && snapshot_ts > ts)) | ||
| { | ||
| // if it is not a snapshot read, there is no need to return | ||
| // deleted keys. | ||
| // If it is a snapshot read and the latest version is newer than | ||
| // snapshot read ts, we need to backfill deleted key since there | ||
| // might be visible archive version of this key for snapshot ts. | ||
| // The caller will decide if reading archive table is necessary | ||
| // based on the deleted key version. | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Get file size to understand how to read it safely
wc -l store_handler/data_store_service_client_closure.cppRepository: eloqdata/tx_service
Length of output: 119
🏁 Script executed:
# View the disputed lines 1317-1328 with context
sed -n '1300,1350p' store_handler/data_store_service_client_closure.cpp | cat -nRepository: eloqdata/tx_service
Length of output: 2404
🏁 Script executed:
# Also view the similar pattern at lines 219-220 mentioned in the review
sed -n '210,230p' store_handler/data_store_service_client_closure.cpp | cat -nRepository: eloqdata/tx_service
Length of output: 1105
🏁 Script executed:
# Search for function names containing the target lines to understand context
rg -nP 'LoadRangeSliceCallback|FetchRecordCallback' store_handler/data_store_service_client_closure.cpp | head -20Repository: eloqdata/tx_service
Length of output: 289
Critical: Snapshot filtering condition is inverted.
The condition snapshot_ts > ts at line 1319 contradicts both the comment and MVCC semantics:
- Comment states: "the latest version is newer than snapshot read ts" (implies
ts > snapshot_ts) - Code checks:
snapshot_ts > ts(opposite condition) - MVCC rule: A snapshot at time S should skip records with
ts > S(records newer than snapshot) - Confirming pattern (lines 219-220): Uses
snapshot_read_ts_ < rec_ts_to detect when a record is too new and requires archive fetch
The condition should be ts > snapshot_ts instead of snapshot_ts > ts to correctly skip records newer than the snapshot timestamp.
🤖 Prompt for AI Agents
In @store_handler/data_store_service_client_closure.cpp around lines 1317-1328,
The snapshot filtering condition is inverted: inside the if-block that checks
(snapshot_ts == 0 && is_deleted) || (snapshot_ts > 0 && snapshot_ts > ts) you
should flip the comparison to (ts > snapshot_ts) so it reads (snapshot_ts == 0
&& is_deleted) || (snapshot_ts > 0 && ts > snapshot_ts); update that condition
in the same if used for skipping deleted/newer records (variables snapshot_ts,
ts, is_deleted) so the code matches the comment and MVCC semantics (skip records
newer than snapshot) and retains the existing continue/backfill behavior.
|
Replaced by PR: #352 |
1 participant
Here are some reminders before you submit the pull request
fixes eloqdb/tx_service#issue_id./mtr --suite=mono_main,mono_multi,mono_basicSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.